Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

18574 - EFT Verification Fixes #1328

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Conversation

ochiu
Copy link
Collaborator

@ochiu ochiu commented Nov 27, 2023

Issue #:
bcgov/entity#18574

Description of changes:

  • fixed statement notification queue event typo in parameter (caused account mailer parsing errors)
  • added dataclass for statement notification event
  • refactor reminder logic to be fully driven by overdue rather than static dates within a month (more accurate and easier for testing/verification)
  • Fix bad queries for EFT payment reminders (joins, timezone issues)
  • Fix to release payments on pay-api when creating an invoice for EFT payment method

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the sbc-pay license (Apache 2.0).

Copy link

sonarcloud bot commented Nov 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

.filter(InvoiceModel.payment_method_code == PaymentMethod.EFT.value,
InvoiceModel.overdue_date.isnot(None),
InvoiceModel.overdue_date.cast(Date) <= current_local_time().date(),
func.date(overdue_datetime) <= current_local_time().date(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix timezone issue

auth_account_ids = [pay_account.auth_account_id for _, pay_account in statement_settings
if pay_account.payment_method == PaymentMethod.EFT.value]

current_app.logger.info(f'Processing {len(auth_account_ids)} EFT accounts for monthly reminders.')
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some debug, and added some additional filtering for EFT payment method for auth_account_ids


@classmethod
def find_most_recent_statement(cls, auth_account_id: str, statement_frequency: str) -> StatementModel:
"""Find all payment and invoices specific to a statement."""
query = db.session.query(StatementModel) \
.join(PaymentAccountModel, PaymentAccountModel.auth_account_id == auth_account_id) \
.join(PaymentAccountModel) \
.filter(PaymentAccountModel.auth_account_id == auth_account_id) \
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed bad join, missing criteria

.filter(StatementModel.frequency == statement_frequency) \
.order_by(StatementModel.to_date.desc())

return query.first()

@classmethod
def determine_to_notify_and_is_due(cls):
def determine_to_notify_and_is_due(cls, invoices: [InvoiceModel]):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored to be properly driven by overdue date

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Merging #1328 (c514355) into main (79924ce) will increase coverage by 0.56%.
Report is 46 commits behind head on main.
The diff coverage is 92.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1328      +/-   ##
==========================================
+ Coverage   91.45%   92.02%   +0.56%     
==========================================
  Files         186      160      -26     
  Lines       11319    10690     -629     
==========================================
- Hits        10352     9837     -515     
+ Misses        967      853     -114     
Flag Coverage Δ
bcolapi ?
eventlistenerqueue ?
payapi 93.68% <92.40%> (-0.05%) ⬇️
paymentjobs 83.32% <94.37%> (+3.10%) ⬆️
paymentreconciliationsqueue ?
reportapi ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
jobs/payment-jobs/tasks/common/dataclasses.py 100.00% <100.00%> (ø)
jobs/payment-jobs/tasks/common/enums.py 100.00% <ø> (ø)
jobs/payment-jobs/tasks/distribution_task.py 97.75% <100.00%> (ø)
...ayment-jobs/tasks/ejv_partner_distribution_task.py 99.15% <100.00%> (+0.02%) ⬆️
jobs/payment-jobs/tasks/ejv_payment_task.py 96.55% <100.00%> (+0.09%) ⬆️
.../payment-jobs/tasks/statement_notification_task.py 79.74% <100.00%> (+47.92%) ⬆️
pay-api/src/pay_api/config.py 99.35% <100.00%> (+<0.01%) ⬆️
pay-api/src/pay_api/models/__init__.py 100.00% <100.00%> (ø)
pay-api/src/pay_api/models/base_model.py 100.00% <100.00%> (ø)
pay-api/src/pay_api/models/eft_credit.py 100.00% <100.00%> (ø)
... and 37 more

... and 36 files with indirect coverage changes

@@ -94,24 +106,24 @@ def publish_statement_notification(pay_account: PaymentAccountModel, statement:
return True


def publish_payment_notification(pay_account: PaymentAccountModel, statement: StatementModel,
is_due: bool, due_date: datetime, emails: str) -> bool:
def publish_payment_notification(info: StatementNotificationInfo) -> bool:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converted params to dataclass to make it cleaner

@@ -72,7 +84,7 @@ def publish_statement_notification(pay_account: PaymentAccountModel, statement:
'emailAddresses': emails,
'accountId': pay_account.auth_account_id,
'fromDate': f'{statement.from_date}',
'toDate:': f'{statement.to_date}',
'toDate': f'{statement.to_date}',
Copy link
Collaborator Author

@ochiu ochiu Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed typo in params causing failure on the account mailer

@@ -60,6 +60,11 @@ def apply_credit(self, invoice: Invoice) -> None:
self.create_receipt(invoice=invoice_model, payment=payment).save()
self._release_payment(invoice=invoice)

def complete_post_invoice(self, invoice: Invoice, invoice_reference: InvoiceReference) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentally reverted this change in a previous PR, adding back in.

This allow payments to be properly released for EFT when invoices are created.

Copy link
Collaborator

@seeker25 seeker25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far

@ochiu ochiu marked this pull request as ready for review November 27, 2023 17:02
@ochiu ochiu requested a review from Jxio as a code owner November 27, 2023 17:02
@seeker25 seeker25 merged commit 420c8ef into bcgov:main Nov 27, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants